Skip to content

fix(harness): pre-trigger hold keeps its argument mutations#519

Merged
ytallo merged 1 commit into
fix/506-release-runs-original-argsfrom
fix/505-hold-discards-mutations
Jul 17, 2026
Merged

fix(harness): pre-trigger hold keeps its argument mutations#519
ytallo merged 1 commit into
fix/506-release-runs-original-argsfrom
fix/505-hold-discards-mutations

Conversation

@ytallo

@ytallo ytallo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Fixes #505.

A harness::hook::pre-trigger hook returning decision: "hold" together with mutations.arguments had the mutations silently discarded, so a released call executed pre-hold arguments — the approval-gate pattern (hold AND stamp validated context) was impossible.

Root cause

Two compounding drops:

  1. parse_output (harness/src/hooks/runner.rs) mapped decision:"hold" to the data-less HookOutcome::Hold — anything under mutations was dropped at parse time.
  2. PreTriggerOutcome::Hold carried no arguments, and the resolve {action:"execute"} path (harness/src/deferred.rs) recovered arguments from the transcript — the model's original call.

Fix

  • HookOutcome::Hold(HookMutations) — hold parses mutations via a shared parse_mutations helper.
  • run_pre_trigger folds the holding hook's rewrite into the accumulated chain arguments and returns them in PreTriggerOutcome::Hold.
  • New durable CallCheckpoint.arguments populated when the call parks (old records stay deserializable); the post-trigger hold park checkpoints effective args too.
  • The execute release prefers checkpointed arguments; transcript recovery remains only as a legacy fallback. filesystem_scope::inject is re-stamped at release so a hook rewrite can never widen scope.

Verification

Note for review

Overlaps with the #506 fix branch (fix/506-release-runs-original-args): both persist held arguments on the call checkpoint but choose different release semantics (this branch: execute checkpointed args; #506's: resume the remaining chain after the holder, per harness.md). Suggest merging #506's mechanism and grafting this branch's hold-parses-mutations onto it, reconciling the checkpoint field (arguments vs held_arguments).

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0638dccb-8107-4bbe-8e83-87a6987061b9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/505-hold-discards-mutations

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@vercel

vercel Bot commented Jul 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
workers Ready Ready Preview, Comment Jul 16, 2026 7:02pm
workers-tech-spec Ready Ready Preview, Comment Jul 16, 2026 7:02pm

Request Review

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

skill-check — worker

0 verified, 44 skipped (no docs/).

Layer Result
structure
vale
ai
render

Four for four. Nicely done.

A pre-trigger hook returning decision:"hold" WITH mutations had them
discarded at parse (parse_output mapped "hold" to a data-less outcome),
so the approval-gate pattern (hold AND stamp validated context) was
impossible. HookOutcome::Hold now carries HookMutations via a shared
parse_mutations helper, and run_pre_trigger folds the holding hook's
argument rewrite into the effective arguments before parking. Those args
ride the call checkpoint added for #506 (fix/506-release-runs-original-args,
which this is based on), so the resolve(execute) release runs with the
gate's mutation.

Unquarantines C-E2E-505, now a regression gate.

Fixes #505
@ytallo
ytallo force-pushed the fix/505-hold-discards-mutations branch from e0ac05b to 9692f5a Compare July 16, 2026 19:01
@ytallo
ytallo changed the base branch from feat/harness-conformance-e2e to fix/506-release-runs-original-args July 16, 2026 19:02
@ytallo
ytallo merged commit 57d8ba0 into fix/506-release-runs-original-args Jul 17, 2026
41 checks passed
ytallo added a commit that referenced this pull request Jul 20, 2026
A pre-trigger hook returning decision:"hold" WITH mutations had them
discarded at parse (parse_output mapped "hold" to a data-less outcome),
so the approval-gate pattern (hold AND stamp validated context) was
impossible. HookOutcome::Hold now carries HookMutations via a shared
parse_mutations helper, and run_pre_trigger folds the holding hook's
argument rewrite into the effective arguments before parking. Those args
ride the call checkpoint added for #506 (fix/506-release-runs-original-args,
which this is based on), so the resolve(execute) release runs with the
gate's mutation.

Unquarantines C-E2E-505, now a regression gate.

Fixes #505
ytallo added a commit that referenced this pull request Jul 20, 2026
…ated arguments (#520)

* fix(harness): release held calls with hook-mutated arguments

A call held by a pre_trigger hook and released with
harness::function::resolve {action: "execute"} ran the target with the
model's ORIGINAL arguments recovered from the transcript — mutations from
hooks that ran before the holder were dropped, and the remaining chain
never resumed, breaking the harness.md § function::resolve promise that
the pre_trigger chain resumes after the holding hook.

- PreTriggerOutcome::Hold now carries the arguments as mutated up to the
  hold; the loop persists them on the call's checkpoint (held_arguments),
  so they survive the park durably. Post-trigger holds checkpoint the
  fully pre-mutated args for the same reason.
- The execute release path prefers the checkpointed arguments (transcript
  recovery remains the fallback for records written before the field
  existed), resumes the pre_trigger chain AFTER the holder — hooks up to
  and including it already ran, so re-running would double-apply
  mutations and re-hold — and handles a resumed Deny/Hold like the loop.
  The filesystem scope stamp is still re-applied after the resumed chain.
- A holder no longer bound at release runs no further hooks: the chain
  shape changed under the hold and the safe resume point is unknowable.

Fixes #506

* test(conformance): unquarantine C-E2E-506 — regression gate for #506

The fix on this branch makes the scenario pass; --scenario all (and the
per-PR CI job) now executes it. Verified locally: C-E2E-001, C-E2E-002,
C-E2E-506 all pass.

* fix(harness): pre-trigger hold keeps its own argument mutations (#519)

A pre-trigger hook returning decision:"hold" WITH mutations had them
discarded at parse (parse_output mapped "hold" to a data-less outcome),
so the approval-gate pattern (hold AND stamp validated context) was
impossible. HookOutcome::Hold now carries HookMutations via a shared
parse_mutations helper, and run_pre_trigger folds the holding hook's
argument rewrite into the effective arguments before parking. Those args
ride the call checkpoint added for #506 (fix/506-release-runs-original-args,
which this is based on), so the resolve(execute) release runs with the
gate's mutation.

Unquarantines C-E2E-505, now a regression gate.

Fixes #505

* test(integration): retry empty descendant PID reads
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant